-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix unnamed lifetime spans #125234
Fix unnamed lifetime spans #125234
Conversation
- More than one unnamed lifetimes gave incorrect spans when lookup was done by name (since they both had name "'_"). Use DefId instead to get correct spans.
rustbot has assigned @compiler-errors. Use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment
compiler/rustc_hir/src/hir.rs
Outdated
@@ -571,6 +571,10 @@ impl<'hir> Generics<'hir> { | |||
self.params.iter().find(|¶m| name == param.name.ident().name) | |||
} | |||
|
|||
pub fn get_with_def_id(&self, id: DefId) -> Option<&GenericParam<'hir>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely not correct if DefId
comes from a different crate. Please make this take a LocalDefId
, or ideally, do the comparison with HirId
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not able to find a way to convert a DefId to a HirId, so I used a LocalDefId instead. The place where I use it already has expect_local() on the binding scope, so presumably the lifetime should also be local. However, I tried to use the same logic for late bound regions, and that caused a panic in a test for trying to unpack a non-local DefId. Is there a way to get the HirId?
☔ The latest upstream changes (presumably #125468) made this pull request unmergeable. Please resolve the merge conflicts. |
@bovinebuddha any updates on this? thanks |
@bovinebuddha |
Fixes issue #125143.
The error message used to mix up two different unnamed lifetimes since they were both assigned the name "'_". Looking up the span using the DefId instead gave correct spans.
I am not sure if a similar fix should be made for any of the other RegionKinds, e.g. ReLateParam, and I am unsure of how to construct a test case which used late bound lifetimes or RePlaceholder.
Maybe the function 'msg_span_from_named_region' should be renamed since it now uses (partially) DefId instead of the name. Also, the name is quite misleading since it is sometimes used to look up unnamed lifetimes...